Make it more clear what an about async fn's returns when referring to what it returns #76765

guswynn · 2020-09-15T22:01:36Z

This is likely not the ONLY place that this happens to be unclear, but we can move this fn to rustc_middle or something like that and reuse it if need be, to apply it to more diagnostics

One outstanding question I have is, if the fn returns (), should I make the message more clear (what about fn f() vs fn f() -> (), can you tell those apart in the hir?)

R? @tmandry

@rustbot modify labels +A-diagnostics +T-compiler

guswynn · 2020-09-15T22:31:25Z

ci failed but I want to make sure Im on the right path first

src/test/ui/issues/issue-76547.stderr

compiler/rustc_infer/src/infer/error_reporting/nice_region_error/different_lifetimes.rs

src/test/ui/self/elision/ref-self-async.stderr

estebank · 2020-09-16T22:27:15Z

src/test/ui/issues/issue-76547.stderr

+LL | async fn fut(bufs: &mut [&mut [u8]]) {
+   |                          ---------   -
+   |                          |
+   |                          this parameter and the returned future are declared with different lifetimes...
+LL |     ListFut(bufs).await
+   |             ^^^^ ...but data from `bufs` is held across an await point here
+   |
+note: `async fn`s return an `impl Future<Output={return type}`
+  --> $DIR/issue-76547.rs:19:38
+   |
+LL | async fn fut(bufs: &mut [&mut [u8]]) {
+   |                                      ^


You'll need to handle this case when including the return type snippet to be impl Future<Output = ()> for this case.

tmandry · 2020-09-16T22:56:39Z

src/test/ui/async-await/multiple-lifetimes/ret-impl-trait-one.stderr

@@ -4,8 +4,14 @@ error[E0623]: lifetime mismatch
 LL | async fn async_ret_impl_trait1<'a, 'b>(a: &'a u8, b: &'b u8) -> impl Trait<'a> {
   |                                           ------                ^^^^^^^^^^^^^^
   |                                           |                     |
-   |                                           |                     ...but data from `b` is returned here
-   |                                           this parameter and the return type are declared with different lifetimes...
+   |                                           |                     ...but data from `b` is held across an await point here


Hmm, this wording doesn't really work if the error is in the function signature itself. I'm confused why the span is pointing here, but one way to make the diagnostic less wrong is to remove the word "here". Then we're just telling you what's happening and providing a span that might help.

sounds good, when I find time (hopefully tomorrow) I will dig into these comments!

camelid · 2020-10-16T02:32:24Z

@guswynn Could you address the review comments?

guswynn · 2020-10-17T19:26:28Z

I'll try to get to it this weekend!

…

On Thu, Oct 15, 2020, 7:32 PM Camelid ***@***.***> wrote: @guswynn <https://github.com/guswynn> Could you address the review comments? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#76765 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABJHNDZL2TVYWNFLEGXO4NLSK6WEPANCNFSM4RN2XERA> .

guswynn · 2020-10-17T22:26:37Z

Updated to the comments.
I have 2 questions

@tmandry mentioned in Make it more clear what an about async fn's returns when referring to what it returns #76765 (comment) that the message this works off may be unclear in some cases, should that a different issue?
wording-wise, is it ok in rust-lang to prepend a span_label msg with "note: "?

...iler/rustc_infer/src/infer/error_reporting/nice_region_error/.different_lifetimes.rs.rustfmt

JohnCSimon · 2020-11-03T05:16:22Z

ping from triage - @guswynn can you mark the 'change requested' comments from camelid as addressed? is this PR ready for review?

guswynn · 2020-11-03T05:29:15Z

@JohnCSimon I believe I marked the comment as resolved, I am.not sure how to clear the request changes.
The pr is ready to review!

tmandry · 2020-11-05T00:03:34Z

compiler/rustc_infer/src/infer/error_reporting/nice_region_error/util.rs

@@ -85,6 +85,60 @@ impl<'a, 'tcx> NiceRegionError<'a, 'tcx> {
        })
    }

+    pub(super) fn future_return_type(


tmandry · 2020-11-05T00:04:56Z

Sorry, this fell off the radar. I think it is fine to merge. Thanks!!

@bors r+ rollup

bors · 2020-11-05T00:04:57Z

📌 Commit 20e032e has been approved by tmandry

camelid · 2020-11-05T07:28:53Z

Here's the diff:

-	error[E0623]: lifetime mismatch
-	  --> $DIR/issue-76547.rs:20:13
+	error: lifetime may not live long enough
+	  --> $DIR/issue-76547.rs:19:14
	   |
	LL | async fn fut(bufs: &mut [&mut [u8]]) {
-	   |                          ---------   -
-	   |                          |           |
-	   |                          |           this `async fn` implicitly returns an `impl Future<Output = ()>`
-	   |                          this parameter and the returned future are declared with different lifetimes...
-	LL |     ListFut(bufs).await
-	   |             ^^^^ ...but data from `bufs` is held across an await point here
+	   |              ^^^^  -     - let's call the lifetime of this reference `'2`
+	   |              |     |
+	   |              |     let's call the lifetime of this reference `'1`
+	   |              assignment requires that `'1` must outlive `'2`
	
-	error[E0623]: lifetime mismatch
-	  --> $DIR/issue-76547.rs:34:14
+	error: lifetime may not live long enough
+	  --> $DIR/issue-76547.rs:33:15
	   |
	LL | async fn fut2(bufs: &mut [&mut [u8]]) -> i32 {
-	   |                           ---------      ---
-	   |                           |              |
-	   |                           |              this `async fn` implicitly returns an `impl Future<Output = i32>`
-	   |                           this parameter and the returned future are declared with different lifetimes...
-	LL |     ListFut2(bufs).await
-	   |              ^^^^ ...but data from `bufs` is held across an await point here
+	   |               ^^^^  -     - let's call the lifetime of this reference `'2`
+	   |               |     |
+	   |               |     let's call the lifetime of this reference `'1`
+	   |               assignment requires that `'1` must outlive `'2`
	
	error: aborting due to 2 previous errors
	

-	For more information about this error, try `rustc --explain E0623`.

guswynn · 2020-11-05T15:03:17Z

I'll try to fix the rebase in the next few days when I find time

guswynn · 2020-11-05T15:04:48Z

Here's the diff:

-	error[E0623]: lifetime mismatch
-	  --> $DIR/issue-76547.rs:20:13
+	error: lifetime may not live long enough
+	  --> $DIR/issue-76547.rs:19:14
	   |
	LL | async fn fut(bufs: &mut [&mut [u8]]) {
-	   |                          ---------   -
-	   |                          |           |
-	   |                          |           this `async fn` implicitly returns an `impl Future<Output = ()>`
-	   |                          this parameter and the returned future are declared with different lifetimes...
-	LL |     ListFut(bufs).await
-	   |             ^^^^ ...but data from `bufs` is held across an await point here
+	   |              ^^^^  -     - let's call the lifetime of this reference `'2`
+	   |              |     |
+	   |              |     let's call the lifetime of this reference `'1`
+	   |              assignment requires that `'1` must outlive `'2`
	
-	error[E0623]: lifetime mismatch
-	  --> $DIR/issue-76547.rs:34:14
+	error: lifetime may not live long enough
+	  --> $DIR/issue-76547.rs:33:15
	   |
	LL | async fn fut2(bufs: &mut [&mut [u8]]) -> i32 {
-	   |                           ---------      ---
-	   |                           |              |
-	   |                           |              this `async fn` implicitly returns an `impl Future<Output = i32>`
-	   |                           this parameter and the returned future are declared with different lifetimes...
-	LL |     ListFut2(bufs).await
-	   |              ^^^^ ...but data from `bufs` is held across an await point here
+	   |               ^^^^  -     - let's call the lifetime of this reference `'2`
+	   |               |     |
+	   |               |     let's call the lifetime of this reference `'1`
+	   |               assignment requires that `'1` must outlive `'2`
	
	error: aborting due to 2 previous errors
	

-	For more information about this error, try `rustc --explain E0623`.

Looks like the test I ADDED no longer triggers the codepath, so this may be more involved to figure out

JohnTitor · 2020-11-05T16:35:48Z

@guswynn We have a builder that enables nll compare mode so you should tweak the test like https://github.com/rust-lang/rust/pull/78268/files#diff-4e99585b86f1234b2e3f288f2e63e8dde1f23834ba26bc79e69d2aace14fd238.

estebank · 2020-11-06T02:40:57Z

./x.py test src/test/ui --stage 1 --keep-stage 1 --bless --compare-mode=nll

guswynn · 2020-11-08T22:55:03Z

@estebank @JohnTitor are you saying that this has rebased onto something turning on a new nll mode? and in the test I should just turn it off?

estebank · 2020-11-09T18:02:39Z

We are saying that in CI tests check for nll enabled output, but locally by default it doesn't.

Running ./x.py test src/test/ui --stage 1 --keep-stage 1 --bless --compare-mode=nll will bless the nll output files and make it pass CI.

guswynn · 2020-11-10T00:30:40Z

@estebank thats a fantastic command! Added the change! should I make a follow up issue to fix the message for compare-mode=nll? (I am not sure why its different, nll has been stable for ages?)

src/test/ui/issues/issue-76547.rs

tmandry · 2020-11-10T01:43:58Z

@estebank thats a fantastic command! Added the change! should I make a follow up issue to fix the message for compare-mode=nll? (I am not sure why its different, nll has been stable for ages?)

Hm yeah, not sure either, it can't hurt to file an issue.

Co-authored-by: Camelid <camelidcamel@gmail.com>

tmandry · 2020-11-10T01:44:13Z

@bors r+ rollup

bors · 2020-11-10T01:44:14Z

📌 Commit 6be4847 has been approved by tmandry

bors · 2020-11-10T01:44:15Z

🌲 The tree is currently closed for pull requests below priority 100, this pull request will be tested once the tree is reopened

…as-schievink Rollup of 14 pull requests Successful merges: - rust-lang#76765 (Make it more clear what an about async fn's returns when referring to what it returns) - rust-lang#78574 (Use check-pass instead of build-pass in regions ui test suite) - rust-lang#78669 (Use check-pass instead of build-pass in some consts ui test suits) - rust-lang#78847 (Assert that a return place is not used for indexing during integration) - rust-lang#78854 (Workaround for "could not fully normalize" ICE ) - rust-lang#78875 (rustc_target: Further cleanup use of target options) - rust-lang#78887 (Add comments to explain memory usage optimization) - rust-lang#78890 (comment attribution fix) - rust-lang#78896 (Clarified description of write! macro) - rust-lang#78897 (Add missing newline to error message of the default OOM hook) - rust-lang#78898 (add regression test for rust-lang#78892) - rust-lang#78908 ((rustdoc) [src] link for types defined by macros shows invocation, not defintion) - rust-lang#78910 (Fix links to stabilized versions of some intrinsics) - rust-lang#78912 (Add macro test for min-const-generics) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

rust-highfive assigned tmandry Sep 15, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 15, 2020

guswynn force-pushed the async_return branch from 8190bd7 to 9f2f83b Compare September 15, 2020 22:02

rustbot added A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 15, 2020

jyn514 added the A-async-await Area: Async & Await label Sep 15, 2020

tmandry reviewed Sep 15, 2020

View reviewed changes

src/test/ui/issues/issue-76547.stderr Outdated Show resolved Hide resolved

guswynn force-pushed the async_return branch from d54aad9 to 10cc864 Compare September 16, 2020 17:26

estebank reviewed Sep 16, 2020

View reviewed changes

compiler/rustc_infer/src/infer/error_reporting/nice_region_error/different_lifetimes.rs Outdated Show resolved Hide resolved

estebank reviewed Sep 16, 2020

View reviewed changes

src/test/ui/self/elision/ref-self-async.stderr Outdated Show resolved Hide resolved

estebank reviewed Sep 16, 2020

View reviewed changes

tmandry reviewed Sep 16, 2020

View reviewed changes

camelid added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 16, 2020

guswynn force-pushed the async_return branch 2 times, most recently from 31a944d to 3d66d3b Compare October 17, 2020 22:24

camelid requested changes Oct 17, 2020

View reviewed changes

...iler/rustc_infer/src/infer/error_reporting/nice_region_error/.different_lifetimes.rs.rustfmt Outdated Show resolved Hide resolved

Make it more clear when complaining about async fn's return types

20e032e

guswynn force-pushed the async_return branch from 3d66d3b to 20e032e Compare October 17, 2020 22:58

camelid added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 3, 2020

tmandry reviewed Nov 5, 2020

View reviewed changes

bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Nov 5, 2020

add nll compare mode stderr file

aaf06d8

camelid reviewed Nov 10, 2020

View reviewed changes

src/test/ui/issues/issue-76547.rs Outdated Show resolved Hide resolved

Update src/test/ui/issues/issue-76547.rs

6be4847

Co-authored-by: Camelid <camelidcamel@gmail.com>

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 10, 2020

guswynn mentioned this pull request Nov 10, 2020

What does compare-mode=nll do these days? #78915

Closed

jonas-schievink mentioned this pull request Nov 10, 2020

Rollup of 14 pull requests #78920

Merged

bors merged commit 9596e34 into rust-lang:master Nov 11, 2020

rustbot added this to the 1.49.0 milestone Nov 11, 2020

guswynn mentioned this pull request Dec 17, 2020

Improve lifetime name annotations for closures & async functions #76468

Merged

guswynn deleted the async_return branch February 19, 2021 21:34

sjakobi mentioned this pull request Mar 26, 2021

async fn can't handle multiple lifetimes in a slice of slices #76547

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make it more clear what an about async fn's returns when referring to what it returns #76765

Make it more clear what an about async fn's returns when referring to what it returns #76765

guswynn commented Sep 15, 2020 •

edited

Loading

guswynn commented Sep 15, 2020

estebank Sep 16, 2020

tmandry Sep 16, 2020

guswynn Sep 16, 2020

camelid commented Oct 16, 2020

guswynn commented Oct 17, 2020 via email

guswynn commented Oct 17, 2020

JohnCSimon commented Nov 3, 2020

guswynn commented Nov 3, 2020

tmandry Nov 5, 2020

tmandry commented Nov 5, 2020

bors commented Nov 5, 2020

camelid commented Nov 5, 2020

guswynn commented Nov 5, 2020

guswynn commented Nov 5, 2020

JohnTitor commented Nov 5, 2020

estebank commented Nov 6, 2020

guswynn commented Nov 8, 2020

estebank commented Nov 9, 2020

guswynn commented Nov 10, 2020 •

edited

Loading

tmandry commented Nov 10, 2020

tmandry commented Nov 10, 2020

bors commented Nov 10, 2020

bors commented Nov 10, 2020

Make it more clear what an about async fn's returns when referring to what it returns #76765

Make it more clear what an about async fn's returns when referring to what it returns #76765

Conversation

guswynn commented Sep 15, 2020 • edited Loading

guswynn commented Sep 15, 2020

estebank Sep 16, 2020

Choose a reason for hiding this comment

tmandry Sep 16, 2020

Choose a reason for hiding this comment

guswynn Sep 16, 2020

Choose a reason for hiding this comment

camelid commented Oct 16, 2020

guswynn commented Oct 17, 2020 via email

guswynn commented Oct 17, 2020

JohnCSimon commented Nov 3, 2020

guswynn commented Nov 3, 2020

tmandry Nov 5, 2020

Choose a reason for hiding this comment

tmandry commented Nov 5, 2020

bors commented Nov 5, 2020

camelid commented Nov 5, 2020

guswynn commented Nov 5, 2020

guswynn commented Nov 5, 2020

JohnTitor commented Nov 5, 2020

estebank commented Nov 6, 2020

guswynn commented Nov 8, 2020

estebank commented Nov 9, 2020

guswynn commented Nov 10, 2020 • edited Loading

tmandry commented Nov 10, 2020

tmandry commented Nov 10, 2020

bors commented Nov 10, 2020

bors commented Nov 10, 2020

guswynn commented Sep 15, 2020 •

edited

Loading

guswynn commented Nov 10, 2020 •

edited

Loading